Skip to content

Replace rowGrouper with getRowGroupKey, rename groupIdGetter to getGroupId - #4159

Merged
nstepien merged 17 commits into
mainfrom
replace-rowGrouper-with-getRowGroupKey
Oct 23, 2026
Merged

nstepien merged 17 commits into
mainfrom
replace-rowGrouper-with-getRowGroupKey

Conversation

@nstepien

@nstepien nstepien commented Aug 29, 2026 •

Copy link
Copy Markdown
Collaborator

TreeDataGrid no longer asks for a function that groups a set of rows into a Record<string, readonly R[]>. Instead, getRowGroupKey(row, columnKey) returns the group key of a single row, and the grid builds the group tree itself.

-  rowGrouper: (rows: readonly R[], columnKey: string) => Record<string, readonly R[]>;
+  getRowGroupKey?: Maybe<(row: R, columnKey: string) => string>;

The new prop is optional. It defaults to String(row[columnKey]), so grouping now only requires groupBy, expandedGroupIds, and onExpandedGroupIdsChange:

 <TreeDataGrid
   groupBy={['country', 'year']}
-  rowGrouper={rowGrouper}
   expandedGroupIds={expandedGroupIds}
   onExpandedGroupIdsChange={setExpandedGroupIds}
 />
-
-function rowGrouper(rows: readonly Row[], columnKey: string) {
-  return Object.groupBy(rows, (row) => row[columnKey]);
-}

Pass it to group on something other than the raw cell value, e.g., bucketing dates by year.

Implementation

  • Rows are grouped in a single pass: each row walks the groupBy keys and is added to every group along its path, so a group's childRows holds all of its descendant rows, same as what rowGrouper used to receive at each level. startRowIndex and the total row count are then computed in one pass over the groups.
  • isGroupRow is now a module-level function backed by a module-level WeakSet, since group rows are only ever created by TreeDataGrid. It no longer changes when the rows are flattened, so it's out of four dependency arrays.
  • React Compiler now compiles TreeDataGrid. On main, it skipped the component because isGroupRow was declared after a return.
  • Removed the now-dead GroupBy, GroupByMap, and ReadonlyGroupByMap types, and isReadonlyArray.

As on main, grouping and flattening are separate memos, so expanding or collapsing a group doesn't regroup the rows, and flattening skips collapsed groups. Grouping depends on rows, groupBy, and getRowGroupKey, so the README now also asks to define groupBy outside the component or memoize it, like getRowGroupKey.

Fix for large groups

Expanding a group threw a RangeError when the group had more rows than browsers accept as function arguments, because its rows were added with push(...rows). That's about 124k rows in Chromium, 500k in Firefox, and 638k in WebKit, and the bug is also on main. The rows are now pushed one by one, and a new test expands a group of a million rows and checks that its first row is rendered.

Breaking changes

groupIdGetter is renamed to getGroupId, as function props are moving to getX names.

Groups are now ordered by the first row that belongs to them. Object.entries() previously sorted integer-like group keys numerically, so a numeric group key over unsorted rows will now come out in row order instead of ascending. A new test covers this, and the row grouping demo now sorts its rows by the grouped columns, in order, so its groups stay sorted at every level.

Notes

vp run ci passes. The existing TreeDataGrid tests now run against the default getRowGroupKey, with dedicated tests covering a custom one and the group order. Visual tests were not run locally; test/visual/treeGrid.test.tsx groups rows that are already in order, so the screenshot is expected to be unchanged.

🤖 Generated with Claude Code

nstepien and others added 2 commits August 29, 2026 02:10
`TreeDataGrid` no longer asks for a function that groups a set of rows into
a `Record<string, readonly R[]>`. Instead, `getRowGroupKey(row, columnKey)`
returns the group key of a single row, and the grid builds the group tree
itself in a single pass over the rows.

Grouping and flattening are now done in one recursion which also returns the
fully-expanded row count, so `startRowIndex` and `aria-rowcount` keep their
previous meaning.

Note: groups are now ordered by the first row that belongs to them, whereas
`Object.entries()` used to sort integer-like group keys numerically.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RQUA3cWdaFLZSQwKAGCC3x
It defaults to `String(row[columnKey])`, which is what most grids need, so
grouping now only requires `groupBy`, `expandedGroupIds` and
`onExpandedGroupIdsChange`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RQUA3cWdaFLZSQwKAGCC3x
@codecov-commenter

codecov-commenter commented Aug 29, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.95%. Comparing base (f1dfede) to head (ec3a181).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4159      +/-   ##
==========================================
+ Coverage   98.58%   98.95%   +0.36%     
==========================================
  Files          46       46              
  Lines        1985     2017      +32     
  Branches      750      787      +37     
==========================================
+ Hits         1957     1996      +39     
+ Misses         28       21       -7     
Files with missing lines Coverage Δ
src/TreeDataGrid.tsx 98.58% <100.00%> (+4.14%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nstepien and others added 2 commits August 29, 2026 02:17
Grouping the rows only depends on `rows`, `groupBy` and `getRowGroupKey`,
while flattening them depends on the expanded state. Keeping both in a single
`useMemo` meant that expanding or collapsing a group re-grouped every row.

`startRowIndex` and the total row count do not depend on the expanded state
either, so they are computed while grouping, and collapsed groups no longer
have to be traversed when the rows are flattened.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RQUA3cWdaFLZSQwKAGCC3x
Group rows are only ever created by `TreeDataGrid`, so a single set can be
shared by every instance instead of being rebuilt on every flatten. That makes
`isGroupRow` a stable module-level function, which drops it from four
dependency arrays and lets the `rows` memo return the rows alone.

`rowHeight` is no longer recomputed when a group is expanded or collapsed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RQUA3cWdaFLZSQwKAGCC3x
@nstepien nstepien self-assigned this Aug 30, 2026
nstepien and others added 7 commits September 4, 2026 13:08
…th-getRowGroupKey

# Conflicts:
#	README.md
#	src/TreeDataGrid.tsx
Add a test that integer-like group keys keep the order of their first row,
instead of being sorted numerically like `Object.entries()` used to.

The row grouping demo only sorted its rows by country, so its year groups
would have come out in random order. Sort the rows by year too.

Add a performance note to `getRowGroupKey`, as a new function identity
groups all the rows again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The groups are only mutated while the rows are being grouped, so build them
with a mutable type and return them from the `useMemo` as a `ReadonlyMap`
of readonly groups, like `useCalculatedColumns` does with its columns.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The `useMemo` already depends on the rows, so it can return the row count
when there is nothing to group too, instead of falling back to
`rawRows.length` on every render.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Function props are moving to `getX` names, which `getRowGroupKey` already
follows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@nstepien nstepien changed the title Replace rowGrouper with getRowGroupKey Replace rowGrouper with getRowGroupKey, rename groupIdGetter to getGroupId Oct 23, 2026
@nstepien
nstepien marked this pull request as ready for review October 23, 2026 01:15
@nstepien

nstepien commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

@amanmahajan7 any issues with replacing rowGrouper with the optional getRowGroupKey prop?
groupIdGetter is also renamed to getGroupId.

I also want to rename rowKeyGetter to getRowKey in a future PR, so this is a precursor to it.

nstepien and others added 2 commits October 23, 2026 17:51
…th-getRowGroupKey

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pushing the rows of an expanded group with `push(...childRows)` throws a
RangeError once the group has more rows than browsers accept as function
arguments, so push them one by one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/TreeDataGrid.tsx
rows: readonly NoInfer<R>[],
columnKey: string
) => Record<string, readonly NoInfer<R>[]>;
getRowGroupKey?: Maybe<(row: NoInfer<R>, columnKey: string) => string>;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it as flexible as rowGrouper? Is there a case where users may want to have custom grouping logic?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only "advantage" is that you could control the order of rows in a group with rowGrouper, but I think it's better if we preserve the original row order.

We previously used Object.entries which did not preserve the row order anyway.
See the "Breaking changes" section in the PR description.

We can add a new sortGroupRows prop or other props if we ever need to.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked Claude to check, if you think none of them can be an issue then we can merge it.

For completeness, here's what rowGrouper allowed that getRowGroupKey no longer does, so we have it on record:

Previously possible Now Workaround
Custom group order (e.g., groups always A→Z, by count, or by an aggregate, regardless of row order). Object.entries only reordered integer-like keys, so for string keys the order returned by rowGrouper was kept. This also meant a rowGrouper that sorted its keys kept groups in place when rows were sorted with sortColumns. Groups follow the order of their first row, so sorting rows also reorders groups Pre-sort rows by the groupBy columns first
Custom order of rows within a group Rows keep their original order Sort rows
Excluding rows from grouping Every row lands in a group Filter rows first
Groups that depend on other rows (e.g., "Top 3 + Other", price bands based on the data's range). rowGrouper received all the rows of the parent group. getRowGroupKey only sees one row Precompute the keys and look them up in getRowGroupKey
Empty groups (e.g., show every month even with no rows). Worked, though an empty group showed as selected because [].every() is true. A group only exists if a row has its key None

@nstepien nstepien Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good?

For "empty groups", your Claude found no workaround, but we can implement nested rows with a custom behavior already:
https://comcast.github.io/react-data-grid/#/MasterDetail
https://comcast.github.io/react-data-grid/#/TreeView

We can revisit as needed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also considering adding a sort prop on columns, which should simplify a lot of stuff, and resolve some of these concerns.

@amanmahajan7

Copy link
Copy Markdown
Collaborator

I also want to rename rowKeyGetter to getRowGet in a future PR, so this is a precursor to it.

You mean getRowKey?

@nstepien

Copy link
Copy Markdown
Collaborator Author

You mean getRowKey?

Yes, my bad

nstepien and others added 4 commits October 23, 2026 20:16
Destructuring in the `for...of` head made React Compiler skip the component.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Groups are ordered by the first row that belongs to them, so nested groups,
e.g., years under sports, were out of order.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The row count doesn't depend on the expanded state, so it alone didn't show
that the group was flattened.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@nstepien
nstepien added this pull request to stack #4228 October 23, 2026 19:52
@nstepien
nstepien merged commit 0e49dec into main Oct 23, 2026
7 of 8 checks passed
@nstepien
nstepien deleted the replace-rowGrouper-with-getRowGroupKey branch October 23, 2026 20:46
nstepien added a commit that referenced this pull request Oct 28, 2026
Rename `rowKeyGetter` to `getRowKey`

The `DataGrid` and `TreeDataGrid` prop that returns a row's key is now
named `getRowKey`, following the `getX` convention that #4159 started
with `getRowGroupKey` and `getGroupId`. `rowKeyGetter` was the last
`xGetter` prop, and 7.0.0 is the time to rename it. The function's type
and behavior don't change.

### Breaking change

```diff
- <DataGrid rowKeyGetter={getRowKey} … />
+ <DataGrid getRowKey={getRowKey} … />
```

Affects every grid with row selection or keyed rows, in both `DataGrid`
and `TreeDataGrid`. The function's signature is unchanged, and the error
thrown when selection is used without it names the new prop.

### Changes

- Renamed the prop in `DataGridProps` (inherited by
`TreeDataGridProps`), and the internal names that follow it:
`rawRowKeyGetter` → `rawGetRowKey` in `TreeDataGrid`, and
`assertIsValidKeyGetter` → `assertIsValidGetRowKey`, whose error now
reads "Please specify the getRowKey prop to use selection".
- Rewrote the prop's JSDoc as short sentences: a function to get the key
of each row, required for row selection, recommended otherwise because
the key is also used as the `key` prop of the row elements, with a
`@returns` tag for the uniqueness requirement. The README paragraph
mirrors the first two sentences, and its callouts keep the
recommendation and the memoization advice.
- Replaced the module-level `rowKeyGetter` functions in the website
demos, browser tests, and visual tests, which all returned `row.id`,
with one generic `getRowKey` helper per tree: `website/utils.tsx` for
the demos and `test/browser/utils.tsx` for the tests. Its key type is
inferred from the row type, so the grids' `K` generic is unaffected;
`rowSelection.test.tsx` spies on `getRowKey<number>`, and the inline
`(row) => row.id` in `renderCell.test.tsx` uses the helper too.
- Renamed the README mentions, examples, and the prop's heading. No
README links pointed at the old heading's anchor.
- Apart from the shared helpers, the diff is a pure rename. #4240 and
#4236 also touch code that reads this prop; whichever lands second needs
a mechanical rebase.
- The prop stays optional, row indexes remain the fallback key, and the
`K` generic stays.

### Verification

`vp run ci` passed in full on the final commit. An earlier run had one
Firefox click timeout in `TreeDataGrid.test.tsx` ("cell navigation in a
treegrid") under machine load, and that file passed when rerun alone
with `vp test test/browser/TreeDataGrid.test.tsx --project "*firefox*"`.
No tests were added; the existing selection and key tests cover the
renamed prop. After `vp pack`, a scratch consumer importing from
`lib/index.js` type-checked `getRowKey` on `DataGrid` and
`TreeDataGrid`, and a `@ts-expect-error` confirmed that `rowKeyGetter`
is rejected, both with and without `exactOptionalPropertyTypes`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
nstepien added a commit that referenced this pull request Oct 29, 2026
…Row` (#4263)

Since #4159, `getRowGroupKey` and `getGroupId` return strings, so
`TreeDataGrid` only ever produces string group keys and IDs, but the
types still said `unknown`. Apps had to cast `groupKey as string`, or
keep `expandedGroupIds` in a `ReadonlySet<unknown>` that could hold
values the grid never produces. This types them all as `string`, and
exports `GroupRow`, which already appeared in public types
(`RenderGroupCellContentProps.row`, `RowHeightArgs`) but couldn't be
named.

This is the first PR of a stack of four: #4264 builds on it, and #4272
renames `GroupRow` to `RowGroup`.

### Breaking change

```diff
- const [expandedGroupIds, setExpandedGroupIds] = useState((): ReadonlySet<unknown> => new Set());
+ const [expandedGroupIds, setExpandedGroupIds] = useState((): ReadonlySet<string> => new Set());
```

`expandedGroupIds`, `onExpandedGroupIdsChange`, `GroupRow.groupKey`,
`GroupRow.parentId`, and `RenderGroupCellContentProps.groupKey` are
typed with `string` instead of `unknown`, which is what `getRowGroupKey`
and `getGroupId` produce. `GroupRow` is now exported.

### Changes

- `GroupRow.parentId` is `string | undefined` (`undefined` on the first
level) and `GroupRow.groupKey` is `string`; every `GroupRow` member now
has JSDoc, checked against `expandGroups` and `setStartRowIndexes`.
- `RenderGroupCellContentProps.groupKey` is `string`; #4264 then removes
it in favor of `row.groupKey`.
- The internal `toggleGroup` parameters of `TreeDataGrid`, `GroupedRow`,
and `GroupCell` are `string`, and `renderToggleGroup` no longer casts
`groupKey as string`.
- `GroupRow` is exported from `src/index.ts`; its README entry is no
longer "(internal)" and sits next to `RenderGroupCellContentProps`, and
the `RenderGroupCellContentProps` and `RowHeightArgs` entries link to
it.
- The `ReadonlySet<unknown>` state in the row grouping demo and in
`TreeDataGrid.test.tsx` is now `ReadonlySet<string>`. The `new Set()`
literals passed to `expandedGroupIds` infer `Set<string>` and need no
change.

### Verification

`vp run ci` passes. No tests are added, because the existing code
already type-checks the new types: `setup()` in `TreeDataGrid.test.tsx`
and the row grouping demo pass `ReadonlySet<string>` state and its
setter to the grid, `toggleGroup` passes `new Set(expandedGroupIds)` to
`onExpandedGroupIdsChange`, and `renderToggleGroup` renders `groupKey`
without a cast. A consumer type-check of the built `lib/index.d.ts`,
with and without `exactOptionalPropertyTypes`, confirms that
`ReadonlySet<unknown>` state is a type error at `expandedGroupIds` and
that `ReadonlySet<string>` state accepts the `Set<string>` from
`onExpandedGroupIdsChange`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants